Skip to content

[18.0][IMP] subscription_oca : add a setting to choose if the subscription …#1404

Open
cvinh wants to merge 1 commit intoOCA:18.0from
invitu:18.0-imp_subscription_oca_no_auto_start
Open

[18.0][IMP] subscription_oca : add a setting to choose if the subscription …#1404
cvinh wants to merge 1 commit intoOCA:18.0from
invitu:18.0-imp_subscription_oca_no_auto_start

Conversation

@cvinh
Copy link
Copy Markdown

@cvinh cvinh commented Mar 12, 2026

…will start automatically when sale order is confirmed or not

Copy link
Copy Markdown
Contributor

@luisDIXMIT luisDIXMIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review and LGTM!

Copy link
Copy Markdown

@rrebollo rrebollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a technical perspective, LGTM.

Would you be open to adding a test covering the change you introduced to the "standard" feature? Before this PR, my understanding from the code is that every newly created subscription automatically started. Now, with your setting, that won't happen by default.

I think adding a test would be a good safety measure—let's see how it goes. You can ping me then.

Also, would you be so kind to review my #1410 in return?

…will start automatically when sale order is conformed or not
@cvinh cvinh force-pushed the 18.0-imp_subscription_oca_no_auto_start branch from 3849115 to 3c883ff Compare April 21, 2026 20:21
@cvinh
Copy link
Copy Markdown
Author

cvinh commented Apr 21, 2026

From a technical perspective, LGTM.

Would you be open to adding a test covering the change you introduced to the "standard" feature? Before this PR, my understanding from the code is that every newly created subscription automatically started. Now, with your setting, that won't happen by default.

I think adding a test would be a good safety measure—let's see how it goes. You can ping me then.

Also, would you be so kind to review my #1410 in return?

tests added

@cvinh
Copy link
Copy Markdown
Author

cvinh commented Apr 21, 2026

I don't understand why tests fail

Copy link
Copy Markdown

@rrebollo rrebollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review. LGTM!

"name": "Subscription management",
"summary": "Generate recurring invoices.",
"version": "18.0.1.0.0",
"version": "18.0.1.1.0",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the version bump should only apply at merge time—though I could be wrong.

@rrebollo
Copy link
Copy Markdown

@cvinh did you rebase? The tests might pass then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants